Repository navigation
refactor(sql-check): drop predicate_effect; keep read-only, bind and permission fixes - #1446
anandgupta42 wants to merge 37 commits into
Conversation
…removes
Analysts are told that a filter which removes nothing usually means the wrong
column, but there was no cheap way to verify it. `sql_check` gains an opt-in
`predicate_effect` check: given an explicit read-only population (`base_sql`,
a SELECT or a table reference) and one to eight boolean predicates, it runs a
single aggregate probe
SELECT COUNT(*), COUNT(*) FILTER (WHERE p1), …, COUNT(*) FILTER (WHERE p1 AND …)
and reports rows kept and removed per predicate plus the combined count,
flagging `this predicate removes no rows at this population`. NULL counts as
not kept and the output says so; an empty population is reported as such.
- No rewriting of the caller's WHERE clause; predicates are validated as bare
expressions through the existing read-only validator and the connection's
dialect, and all supplied SQL goes through the `sql_execute` permission
request before routing.
- Bounded: SQLite, DuckDB and Postgres only; a catalog row estimate above
`max_rows_scanned` (default 5,000,000) for a bare table refuses with a
suggestion to narrow the population; query timeouts return
`skipped: timeout` instead of failing the caller.
- Analyst prompt: the existing "every clause must bite" sentence points at the
check and says a vacuous filter is a reason to profile an alternative
column, not to invent an exclusion.
Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…uts and all-row removal Review follow-ups to the predicate-effect probe: - When the catalog row estimate is unknown (views, SQLite without ANALYZE, unanalysed or partitioned Postgres tables, ambiguous relations) the output says `row_estimate: unknown — probe runs under the query timeout` instead of proceeding silently. - Strict-schema fillers (`max_rows_scanned: 0`, blank predicates, empty `base_sql`) are treated as absent and produce the tool's own message, never a raw schema validation dump. - A predicate that removes every row is flagged; counts over a `LIMIT`ed SELECT population say so. - Postgres branch covered by tests (probe shape, `to_regclass`/`reltuples` guard, unknown-estimate path). - The analyst prompt sentence ends with a period. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configuration
You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
full receipts (20 sessions)
codex ·
|
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 52f0e5ed74
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…nd keeps caller positions Independent-review follow-ups: - A bare-table population is wrapped under the table's own name, so a predicate copied from the caller's WHERE clause (`orders.kind = 'a'`) works; SELECT populations keep the `t` alias and the description says so. - The limited-population note fires only for a top-level `LIMIT`, `FETCH FIRST … ROWS ONLY` or `TOP` on the population statement, not for a LIMIT inside a subquery. - Blank predicate entries are reported as skipped at their original position, so predicate numbers match the caller's array. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 25baadb1c2
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using high effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 25baadb. Configure here.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
… in predicates and populations A review finding of the highest severity tier: on PostgreSQL connections allowed to signal other backends, `pg_terminate_backend(pid)` or `pg_cancel_backend(pid)` passes the read-only validator as a boolean expression. The shared validator now blocks backend-signalling calls (including escaped function names and CR-comment variants) before any query runs; harmless literals, comments, column names and scalar expressions remain accepted. Tests: `rejects backend-signaling predicate <case> before query execution` and the population variant (14 cases). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
…' into feat/analyst-v10-probe
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e9b84ed5e5
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…trol and dblink functions Three review findings of the highest severity tier, each with red-first tests: - The probe executes with the connector's read-only mode, so a user-defined function that writes cannot be reached through a predicate. - `pg_promote` and `pg_reload_conf` are denied in predicates and populations. - `dblink_send_query`, `dblink_connect` and `dblink_connect_u` are denied in predicates and populations. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 3fa390c878
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
Code Review SummaryStatus: No Issues Found | Recommendation: Merge Files Reviewed (10 files)
Previous Review Summaries (8 snapshots, latest commit 5df7cfa)Current summary above is authoritative. Previous snapshots are kept for context only. Previous review (commit 5df7cfa)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (10 files)
Fix these issues in Kilo Cloud Previous review (commit 214e47e)Status: No Issues Found | Recommendation: Merge Files Reviewed (2 files)
Previous review (commit 9ae7d77)Status: No Issues Found | Recommendation: Merge Files Reviewed (3 files)
Previous review (commit 5a69583)Status: 2 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit c9cfe17)Status: 1 Issue Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 2f824cf)Status: 6 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
SUGGESTION
Files Reviewed (2 files)
Fix these issues in Kilo Cloud Previous review (commit 8e56751)Status: 4 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)WARNING
Files Reviewed (8 files)
Fix these issues in Kilo Cloud The three prior critical findings are fixed at the current HEAD. No new inline findings were raised; existing comments were not duplicated. Review based on source and existing tests only; no code or tests were executed. Previous review (commit 3fa390c)Status: 7 Issues Found | Recommendation: Address before merge Overview
Issue Details (click to expand)CRITICAL
WARNING
Files Reviewed (6 files)
Fix these issues in Kilo Cloud Review based on source and existing tests only; no code or tests were executed. Earlier active review comments were not duplicated. Reviewed by gemini-4-argon · Input: 539.9K · Output: 2.4K · Cached: 2.2M Review guidance: REVIEW.md from base branch |
…_effect` - Read-only classification: deny `pg_logical_emit_message` (its non-transactional form is written to WAL immediately and survives the rollback), bare `dblink` (the synchronous form runs its string argument on a remote connection outside the local read-only transaction) and every `pg_advisory_lock` / `pg_try_advisory_lock` / `pg_advisory_xact_lock` variant (session-level locks survive ROLLBACK and the session returns to the pool). Applies to the population and to every predicate; case-insensitive and schema-qualified spellings covered. - Permission: `PermissionNext.ask` evaluated patterns in order and returned a prompt on the first `ask`, so a later pattern with an explicit `deny` would execute once the earlier one was approved. It now evaluates every pattern before publishing the approval request (`upstream_fix`). - Local engines honour `readOnly`: SQLite runs the probe under a scoped `query_only` pragma restored afterwards; DuckDB runs it in a READ ONLY transaction that is rolled back, and refuses to start one while the caller has its own transaction open rather than aborting it. Previously the flag was silently ignored on both engines. - Regression tests for each denied function family, for the deny-before-prompt ordering, and for SQLite/DuckDB read-only enforcement (hidden sequence writes in a view and in a predicate subquery, pragma restoration after failure, and the active-transaction refusal). Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
There was a problem hiding this comment.
All reported issues were addressed across 8 files (changes from recent commits).
Reply with feedback, questions, or to request a fix.
View guided diff | Turn on auto-fix | Re-trigger cubic
…te_effect pg_advisory_unlock, pg_advisory_unlock_shared and pg_advisory_unlock_all release session-level locks held by the pooled connection, and a read-only rollback does not undo that. The denylist now covers every pg_advisory_* / pg_try_advisory_* function; regression cases added for the three unlock variants. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
… the predicate_effect branch Resolves the conflict in `packages/drivers/src/duckdb.ts` by keeping the shared-engine generation tracking from the base and this branch's `readOnly` flag on the engine call.
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 962a10f7-5127-45e8-8bfa-0981b2640611) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9fce7bad1e
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…s in read-only `sql_check` sources The denylist named individual dblink functions, so `dblink_open`/`dblink_fetch`/`dblink_close` (a remote data-modifying cursor whose close commits) passed, and it omitted `pg_wal_replay_pause`/`pg_wal_replay_resume`, which change recovery state on a standby and are not undone by rollback. The list now matches `dblink\w*` and `pg_wal_replay_\w*`. Tests: dblink cursor open/fetch/close (incl. a data-modifying CTE), and both WAL replay controls, each rejected before query execution in plain, schema-qualified and commented spellings. Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 724a32c4-b274-4bd6-bad5-6b51aa6d6aca) |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: ae69002dfd
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…shaped calls - The permission request now includes the probe's AND-combined predicate expression and the fixed catalog lookups (`pg_operator`, `pg_proc`, `pg_type`, `duckdb_functions()`), so rules scoped to a joint condition or to catalog reads apply before anything executes. - Only reserved keywords that cannot name an unquoted function are skipped by call detection. `FILTER` and `OVER` count as clauses only right after a call's closing parenthesis. Unreserved names such as `range`, `rows`, `partition` and `within` are validated as calls, and `range` joins the pure allowlist so the catalog check still refuses a user-defined overload. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: bc7e6804-df80-4a90-a307-f7f4dcdc5880) |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 9055a9102d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…predicate_effect` runs The permission request now mirrors execution: the PostgreSQL `pg_class` row estimate for a bare table is included, and each catalog lookup (operators, functions, casts, `duckdb_functions()`) is requested only when the SQL contains what that lookup checks. The estimate statement is shared between the request and the probe. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: fb21e109-3d2d-435a-a6b8-a0d437d89b9c) |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6b16bdb2c9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…' into feat/analyst-v10-probe
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 4924a910-1cc4-4dd8-b233-d8edd7ff0312) |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
1 similar comment
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 5490811e8d
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
…ect` pattern Permission patterns for the probe are built by one helper. For a bare PostgreSQL table it adds the row-estimate lookup followed by the probe, in the order the executed statement contains them, so a rule spanning the catalog and a predicate applies. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
…' into feat/analyst-v10-probe
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: 8d1dbfe2-4c10-44a9-baa9-56286a935269) |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
2 similar comments
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
👋 This PR was automatically closed by our quality checks. Common reasons:
If you believe this was a mistake, please open an issue explaining your intended contribution and a maintainer will help you. |
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
…eric fixes The mode validated caller-supplied SQL by its text, and each review round found another server-side definition that the text cannot reveal. It was opt-in, off by default and unused, so it is removed with its config key, prompt sentence and tests. Kept, with regression tests: permission requests evaluate every pattern before prompting, PostgreSQL forwards bind parameters on both query paths, SQLite and DuckDB honour `readOnly`, tokenizer fixes for shared read validation, and readable `sql_check` validation errors. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
sql_check predicate_effect reports what each filter removespredicate_effect; keep read-only, bind and permission fixes
Bugbot couldn't run - usage limit reachedBugbot is counted against Cursor usage for this user or team, and this run hit a usage or spend limit. A user or team admin can review and increase usage limits in the Cursor dashboard. (requestId: f6449f42-8c38-403a-a522-a5e037e4a859) |

Issue for this PR
Closes #
Type of change
What does this PR do?
Scope change. This PR originally added a
sql_checkpredicate_effectmode. That mode is removed: every review round found another server-side definition (view bodies, cast functions, operators, catalog reads) that validating the SQL text could not see, it was already opt-in and off by default, and the agent never used it. What remains are the generic fixes that work surfaced, each with its own regression tests.What changed (was, then now):
permission/next.ts).Connector.executedropped bind values on the regular and byte-limited query paths. Now they are forwarded on both.readOnlywas honoured only on PostgreSQL. Now SQLite runs withquery_only(restoring the previous setting afterwards) and DuckDB runs inside a read-only transaction, refusing to start inside a caller's open transaction.sql_checkerrors. Validation failures return a readable message instead of a raw exception.Base. Stacked on
feat/analyst-harness-dab-2026-10(#1441).Behaviour deltas.
sql_checkkeeps its existing checks (fanout,keys,grain,nulls,coverage) with no new parameters or config. SQLite and DuckDB reads issued withreadOnlynow refuse writes. A permission request with a later denied pattern is now denied instead of prompted.How did you verify your code works?
packages/opencode:test/altimate/tools,test/config,test/permission,test/agent/analyst-data-explorer.test.tsand the query-deadline suite.packages/drivers: full suite, including the PostgreSQL bind-forwarding and read-only tests.bun run typecheck, the upstream marker check andgit diff --check.packages/.Screenshots / recordings
Not a UI change.
Checklist
🤖 Generated with Claude Code
Note
Medium Risk
Changes touch SQL execution guards, permission deny semantics, and read-only enforcement across three engines—behavior shifts (stricter reads, multi-pattern denies) are intentional but affect agent SQL tooling paths.
Overview
This PR tightens read-only warehouse execution and
sql_checksafety, plus fixes permission evaluation and PostgreSQL parameter forwarding.Drivers.
ExecuteOptions.readOnlynow applies on SQLite (PRAGMA query_only, restored after each call) and DuckDB (helper runsBEGIN TRANSACTION READ ONLYthenROLLBACK, and refuses nested use inside an open transaction). DuckDB/SQLite drivers passreadOnlythrough to the local engine child. PostgreSQL again forwards bind parameters on both normal and byte-limited streaming query paths.sql_check. Warehouse queries always usereadOnly: truefor SQLite, DuckDB, and Postgres. Read validation is stricter: broader denial of PostgreSQL side-effectpg_*/dblink/ large-object calls, rejection ofU&"..."function-name escapes, and tokenizer fixes so CR-ended comments, Postgres backticks, and Postgres/DuckDBARRAY[...]do not hide writes. Invalid tool args return a clearsql_check invalid inputmessage before connecting.Permissions.
PermissionNext.askevaluates every pattern before showing a prompt; any matching deny wins instead of prompting on an earlier pattern only.Reviewed by Cursor Bugbot for commit e8eafe4. Bugbot is set up for automated code reviews on this repo. Configure here.